Beta Support for XDNA2 Platform#179
Open
Victor-Jung wants to merge 27 commits into
Open
Conversation
…code transformation passes
Victor-Jung
changed the title
Contributor
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds complete XDNA2 (AIE2p) platform support: MLIR data types and passes, node templates/bindings, XDNA2 deployer and platform integration, BF16 Add AIE kernel and CMake wiring, Docker and pinned Python deps, test generation and host test harness, pytest integration, and CI reusable workflow for NPU runner. ChangesXDNA2 Platform & MLIR Codegen
Sequence Diagram(s)sequenceDiagram
participant Deployer as XDNA2Deployer.generateMLIR
participant DevicePass as MLIRObjectFifoPass/MLIRComputeCorePass
participant RuntimePass as MLIRRuntimeSequencePass
participant Kernel as AIE Kernel (eltwise_add)
participant Host as XRT Host (main.cpp)
Deployer->>DevicePass: create device blocks (FIFOs, kernel decl, compute core)
DevicePass-->>Deployer: embed device regions in MLIR
Deployer->>RuntimePass: create runtime_sequence (DMA tasks, seq args)
RuntimePass-->>Deployer: embed runtime sections in MLIR
Host->>Kernel: load xclbin & instructions, start kernel with buffers
Kernel-->>Host: finish and signal
Host->>Deployer: validate outputs against generated golden data
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (16)
DeeployTest/testUtils/testRunner.py (1)
95-114: Validate tiling CLI inputs at parse-time.
--l1/--l3currently accept non-positive values, and--defaultMemLevelaccepts arbitrary strings. Adding argument-level validation avoids late failures in generation passes.Suggested refactor
+def _positive_int(value: str) -> int: + ivalue = int(value) + if ivalue <= 0: + raise argparse.ArgumentTypeError("Value must be a positive integer") + return ivalue + class TestGeneratorArgumentParser(argparse.ArgumentParser): @@ self.add_argument('--l1', metavar = '<size>', dest = 'l1', - type = int, + type = _positive_int, default = None, help = 'Set L1 memory size in bytes (enables tiling if specified).\n') self.add_argument('--l3', metavar = '<size>', dest = 'l3', - type = int, + type = _positive_int, default = None, help = 'Set L3 memory size in bytes.\n') self.add_argument('--defaultMemLevel', metavar = '<level>', dest = 'defaultMemLevel', type = str, + choices = ["L1", "L2", "L3"], default = "L3", help = 'Set default memory level (default: L3)\n')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@DeeployTest/testUtils/testRunner.py` around lines 95 - 114, The CLI accepts invalid tiling inputs; validate them at argument-parse time by replacing the raw int/str types with validation helpers: for '--l1' and '--l3' use a positive-integer validator (e.g., positive_int) that raises argparse.ArgumentTypeError for <=0 values, and for '--defaultMemLevel' restrict values via choices (e.g., ['L1','L3']) or a validator that normalizes and rejects unknown values; update the self.add_argument calls for l1, l3, and defaultMemLevel in testRunner.py accordingly so parsing fails fast with clear messages.DeeployTest/test_platforms.py (1)
128-150: Consider updating the markers summary to includexdna2.The markers summary comment documents available platform markers but doesn't include the newly added
xdna2marker.Suggested addition
# snitch: tests from the Snitch platform (untiled) # snitch_tiled: tests from the Snitch platform (tiled) # siracusa: tests from the Siracusa platform (untiled) # siracusa_tiled: tests from the Siracusa platform (tiled) # siracusa_neureka_tiled: tests from the Siracusa + Neureka platform (tiled) # gap9: tests from the GAP9 platform (untiled) # gap9_tiled: tests from the GAP9 platform (tiled) +# xdna2: tests from the XDNA2 platform🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@DeeployTest/test_platforms.py` around lines 128 - 150, Update the markers summary comment in the Platform markers section of DeeployTest/test_platforms.py to include the new platform marker `xdna2`; specifically add `xdna2: tests from the XDNA2 platform` (or a similarly descriptive phrase) alongside the existing platform marker lines so the comment accurately documents the new `xdna2` marker.CMakeLists.txt (1)
23-24: Add "XDNA2" to the platform property strings list.The
XDNA2platform is handled in the build logic (lines 49-50 and 304-317), but it's not included in theset_property(CACHE platform PROPERTY STRINGS ...)list. This affects CMake GUI/ccmake users who rely on the dropdown for valid platform options.Proposed fix
set(platform MemPool CACHE STRING "Platform (MemPool, SoftHier, QEMU, Siracusa, Siracusa_w_neureka, PULP-Open, GAP9, Generic, Snitch)") -set_property(CACHE platform PROPERTY STRINGS MemPool SoftHier QEMU Siracusa Siracusa_w_neureka PULP-Open GAP9 Generic Snitch) +set_property(CACHE platform PROPERTY STRINGS MemPool SoftHier QEMU Siracusa Siracusa_w_neureka PULP-Open GAP9 Generic Snitch XDNA2)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CMakeLists.txt` around lines 23 - 24, The CMake cache property list for the platform option is missing "XDNA2", causing GUI/ccmake users to not see it as a valid choice; update the set_property(CACHE platform PROPERTY STRINGS ...) call to include "XDNA2" (and keep it consistent with the initial set(platform ...) default if you want it selectable there too) so the platform dropdown contains XDNA2 alongside MemPool, SoftHier, QEMU, Siracusa, Siracusa_w_neureka, PULP-Open, GAP9, Generic, and Snitch.Deeploy/Targets/XDNA2/CodeTransformationPasses/__init__.py (1)
5-7: Consider explicit imports for better IDE support and static analysis.While wildcard imports in
__init__.pyare a common re-export pattern, explicit imports with__all__provide better discoverability and make the public API explicit. This is a minor maintainability suggestion.Optional: Explicit imports alternative
-from Deeploy.Targets.XDNA2.CodeTransformationPasses.MLIRComputeCorePass import * -from Deeploy.Targets.XDNA2.CodeTransformationPasses.MLIRObjectFifoPass import * -from Deeploy.Targets.XDNA2.CodeTransformationPasses.MLIRRuntimeSequencePass import * +from Deeploy.Targets.XDNA2.CodeTransformationPasses.MLIRComputeCorePass import MLIRComputeCorePass +from Deeploy.Targets.XDNA2.CodeTransformationPasses.MLIRObjectFifoPass import MLIRObjectFifoPass +from Deeploy.Targets.XDNA2.CodeTransformationPasses.MLIRRuntimeSequencePass import MLIRRuntimeSequencePass + +__all__ = ['MLIRComputeCorePass', 'MLIRObjectFifoPass', 'MLIRRuntimeSequencePass']🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Deeploy/Targets/XDNA2/CodeTransformationPasses/__init__.py` around lines 5 - 7, Replace the three wildcard imports in __init__.py with explicit imports of the public symbols exported by MLIRComputeCorePass, MLIRObjectFifoPass, and MLIRRuntimeSequencePass (e.g., import the specific classes/functions you intend to re-export) and then define an explicit __all__ list containing those symbol names; update the import lines referencing Deeploy.Targets.XDNA2.CodeTransformationPasses.MLIRComputeCorePass, MLIRObjectFifoPass, and MLIRRuntimeSequencePass to import only the public symbols and set __all__ = [...names...] so IDEs and static analyzers can discover the package API.DeeployTest/testUtils/platformMapping.py (1)
286-287: Chain the exception for better debugging.When re-raising an exception, use
fromto preserve the original traceback. This helps with debugging when the import fails for unexpected reasons.🔧 Proposed fix
- except ImportError: - raise RuntimeError(f"Deployer for platform {platform} is not implemented") + except ImportError as err: + raise RuntimeError(f"Deployer for platform {platform} is not implemented") from err🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@DeeployTest/testUtils/platformMapping.py` around lines 286 - 287, The except ImportError handler in the function that loads deployers currently raises RuntimeError(f"Deployer for platform {platform} is not implemented") without chaining the original ImportError; modify the except block to capture the ImportError (e.g., except ImportError as e:) and re-raise the RuntimeError using exception chaining (raise RuntimeError(f"Deployer for platform {platform} is not implemented") from e) so the original traceback is preserved for debugging.Container/Dockerfile.deeploy-xdna (2)
14-35: Consider adding--no-install-recommendsto reduce image size.Adding
--no-install-recommendscan significantly reduce the Docker image size by avoiding installation of suggested but non-essential packages.📦 Proposed fix
RUN apt-get update && apt-get install -y \ + --no-install-recommends \ software-properties-common \ && add-apt-repository -y ppa:amd-team/xrt \ - && apt-get update && apt-get install -y \ + && apt-get update && apt-get install -y --no-install-recommends \ cmake \🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Container/Dockerfile.deeploy-xdna` around lines 14 - 35, Update the RUN layer that invokes apt-get to add --no-install-recommends to both apt-get install -y invocations in the multi-step RUN command (the line starting with "RUN apt-get update && apt-get install -y \" and the subsequent "&& apt-get update && apt-get install -y \" sequence) so that unnecessary recommended packages are not installed and image size is reduced; keep the existing rm -rf /var/lib/apt/lists/* cleanup as-is.
12-12: Clarify the purpose ofLLVM_INSTALL_DIR="nope".This environment variable with a placeholder value may cause confusion. Consider adding a comment explaining why this is set or whether it's a workaround for a specific issue.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Container/Dockerfile.deeploy-xdna` at line 12, The ENV line setting LLVM_INSTALL_DIR="nope" is ambiguous; update the Dockerfile to either set a meaningful path or add an inline comment explaining why "nope" is used (e.g., as a deliberate sentinel to disable LLVM lookup or to work around a specific build issue). Locate the ENV LLVM_INSTALL_DIR declaration and either replace "nope" with the correct install path or append a short comment describing the reason and any related workarounds or downstream expectations so future maintainers aren’t confused.README_XDNA.md (2)
21-21: Consider clarifying the temporary IRON dependency.This line references a specific developer's path (
/scratch/jungvi/IRON) and an internal workaround. Consider either removing this line for public documentation or adding context about when this workaround is needed vs. the standard flow.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README_XDNA.md` at line 21, Update the README_XDNA.md sentence that embeds a personal developer path by either removing the specific `/scratch/jungvi/IRON` example or replacing it with a generic, documented workaround: describe that mounting an IRON repo is a temporary step only required until Deeploy midend/backend support MLIR, show a generic volume example like `-v /path/to/IRON:/opt/IRON`, and explain the corresponding env var `IRON_OPERATORS_DIR=/opt/IRON/iron/operators` and when users should use it versus the standard flow; ensure the text clearly labels it as a temporary, developer-only workaround.
6-8: Add language specifiers to fenced code blocks.The code blocks are missing language specifiers, which affects syntax highlighting and accessibility. Add
bashorshellto the command blocks.📝 Proposed fix
-``` +```bash docker build -f Container/Dockerfile.deeploy-xdna -t deeploy-xdna:local . ```Apply similar changes to other code blocks at lines 11, 24, 38, and 44.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README_XDNA.md` around lines 6 - 8, Add language specifiers (e.g., bash or shell) to the fenced code blocks that contain shell commands so they get proper syntax highlighting; for example, change the block around the docker build command (`docker build -f Container/Dockerfile.deeploy-xdna -t deeploy-xdna:local .`) to start with ```bash and similarly update the other command/code blocks referenced (lines with commands at the other snippets) to use ```bash or ```shell so all shell snippets in README_XDNA.md are consistently labeled.DeeployTest/generateNetwork_xdna2.py (2)
191-196: Hardcoded Add operation limits reusability.The golden output computation hardcodes elementwise addition, which will break for other operations. The TODO comment acknowledges this should be moved to ONNX4Deeploy.
Consider either:
- Running ONNX inference with BF16-quantized inputs to compute operation-agnostic golden outputs
- Extracting the operation type from the graph and dispatching to the appropriate NumPy function
Would you like me to help design a more generic golden output computation approach?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@DeeployTest/generateNetwork_xdna2.py` around lines 191 - 196, The golden-output computation currently hardcodes elementwise addition using bf16_inputs -> bf16_input_f32 -> golden_f32 and will fail for non-add ops; replace this with an operation-agnostic approach: either run the ONNX graph (using the BF16-quantized inputs bf16_inputs) to produce test_outputs_bf16, or inspect the model graph to extract the root operator(s) and dispatch to the corresponding NumPy routine (e.g., use op_type from nodes and call the appropriate handler) so that test_outputs_bf16 is produced generically instead of summing elements.
129-137: Fix unused loop variable and addstrict=Truefor safety.The
namevariable is unused, andzip()withoutstrict=can silently produce incorrect results if the iterables have different lengths.🔧 Proposed fix
- for index, (name, values) in enumerate(zip(inputs_npz.files, test_inputs_f32)): + for index, (_name, values) in enumerate(zip(inputs_npz.files, test_inputs_f32, strict=True)):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@DeeployTest/generateNetwork_xdna2.py` around lines 129 - 137, In the loop that currently reads "for index, (name, values) in enumerate(zip(inputs_npz.files, test_inputs_f32))", mark the unused filename variable as _ and make zip strict to avoid length-mismatch bugs: change the header to "for index, (_, values) in enumerate(zip(inputs_npz.files, test_inputs_f32, strict=True))" so you still populate inputTypes/inputOffsets with PointerClass(bfloat16_t) as before; no other logic changes needed.Deeploy/Targets/XDNA2/CodeTransformationPasses/MLIRObjectFifoPass.py (1)
47-47: Usenext(iter(...))instead oflist(...)[0].Per static analysis suggestion, this is more efficient as it avoids creating an intermediate list.
♻️ Proposed fix
- firstOutputName = list(outputConstraints.keys())[0] + firstOutputName = next(iter(outputConstraints.keys()))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Deeploy/Targets/XDNA2/CodeTransformationPasses/MLIRObjectFifoPass.py` at line 47, Replace the inefficient list(...) indexing when obtaining the first key from outputConstraints: locate the assignment to firstOutputName in MLIRObjectFifoPass.py and use next(iter(outputConstraints)) (or next(iter(outputConstraints.keys()))) instead of list(outputConstraints.keys())[0] so you avoid creating an intermediate list; update the code where firstOutputName is set accordingly.Deeploy/Targets/XDNA2/CodeTransformationPasses/MLIRComputeCorePass.py (1)
68-88: Assumes uniform tile type for all input/output tensors.Line 70 uses
firstKey's type astileTy, and line 74 creates a singlesubviewTyused for all FIFO acquisitions. This works for the current Add operation where all tensors have identical shapes and types, but will need generalization for operators with mixed tensor types.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Deeploy/Targets/XDNA2/CodeTransformationPasses/MLIRComputeCorePass.py` around lines 68 - 88, The code assumes a single representative tile type (tileTy derived from firstKey) and reuses one subviewTy for all FIFO acquisitions, which breaks when tensors have different types; update the loop so you look up the per-key memref type via mlirBlock.fifoTypes[key] and create a matching subview type for each acquisition (i.e., compute subviewTyPerKey from mlirBlock.fifoTypes[key] before calling aie_d.objectfifo_acquire/objectfifo_subview_access), handling inputs (self.inputTensorKeys) and outputs (self.outputTensorKeys) independently rather than reusing tileTy/subviewTy for every key.Deeploy/Targets/XDNA2/Platform.py (1)
126-138: DuplicatedgetTargetMemoryLevellogic.Both
MemoryXDNA2PlatformandMemoryXDNA2PlatformWrapperimplement identical logic for checking_engine_assignment. Consider extracting to a shared helper function to maintain consistency.♻️ Proposed refactor
def _get_xdna2_target_memory_level(node: gs.Node, default_level_name: str) -> str: """Shared logic for XDNA2 memory level selection.""" if hasattr(node, '_engine_assignment'): engine = node._engine_assignment if isinstance(engine, XDNA2AIECoreEngine) and hasattr(engine, 'preferredMemoryLevel'): return engine.preferredMemoryLevel return default_level_nameThen both classes can call this helper in their
getTargetMemoryLevelmethods.Also applies to: 150-157
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Deeploy/Targets/XDNA2/Platform.py` around lines 126 - 138, Extract the duplicated engine-assignment logic from both MemoryXDNA2Platform.getTargetMemoryLevel and MemoryXDNA2PlatformWrapper.getTargetMemoryLevel into a shared helper (e.g., _get_xdna2_target_memory_level) that accepts the node and the default level name; have each getTargetMemoryLevel call this helper instead of repeating the hasattr/instanceof checks against XDNA2AIECoreEngine and preferredMemoryLevel, returning engine.preferredMemoryLevel when present or defaultTargetMemoryLevel.name otherwise.Deeploy/Targets/XDNA2/Deployer.py (1)
159-171: Tensor type derived from first node only.Line 163 derives
tensorTyfromfirstEb.numElementsand hardcodesBF16Type. This assumes all runtime sequence arguments have the same element count and type. For the current Add-only scope this is fine, but consider documenting this assumption or adding validation when extending to multi-node graphs with varying tensor sizes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Deeploy/Targets/XDNA2/Deployer.py` around lines 159 - 171, The code derives tensorTy from the first block (mlirBlocks[0]) using firstEb.numElements and a hardcoded ir.BF16Type, which assumes every runtime-sequence arg shares the same element count and type; update the runtime-sequence setup in Deployer.py to validate that all blocks in mlirBlocks have matching numElements (and, if applicable, matching element types) before creating tensorTy (or document this assumption clearly), and emit a clear error or fallback if a mismatch is detected so aiex_d.runtime_sequence(_seq) is not created with inconsistent tensor shapes.DeeployTest/Platforms/XDNA2/CMakeLists.txt (1)
29-69: LGTM! Toolchain resolution is well-structured.The fallback chain (cache → environment → Python query) for
LLVM_AIE_INSTALL_DIRandMLIR_AIE_INSTALL_DIRprovides good flexibility.One minor inconsistency:
XRT_INSTALL_DIRisn't declared as a cache variable like the others. Consider addingCACHE PATHfor consistency:-if(NOT XRT_INSTALL_DIR) +set(XRT_INSTALL_DIR "" CACHE PATH "XRT install dir") +if(NOT XRT_INSTALL_DIR)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@DeeployTest/Platforms/XDNA2/CMakeLists.txt` around lines 29 - 69, The XRT_INSTALL_DIR variable should be declared as a cache variable like LLVM_AIE_INSTALL_DIR and MLIR_AIE_INSTALL_DIR for consistent configuration and overrides; change the declaration so XRT_INSTALL_DIR is initialized with set(XRT_INSTALL_DIR "$ENV{XRT_INSTALL_DIR}" CACHE PATH "XRT install dir") (or similar) and preserve the existing fallback logic in the if(NOT XRT_INSTALL_DIR) block that checks ENV{XILINX_XRT} and defaults to "/opt/xilinx/xrt", ensuring the cache variable is used/checked instead of an undeclared plain variable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Deeploy/MLIRDataTypes.py`:
- Around line 187-201: The generate method uses a mutable default
operatorRepresentation = {} which can lead to shared-state bugs; change the
signature of generate (the method named generate in MLIRDataTypes.py) to accept
operatorRepresentation = None (or Optional[Dict]) and inside the method do a
guarded assignment like "if operatorRepresentation is None:
operatorRepresentation = {}" before using it, ensuring you preserve the existing
behavior and return value while avoiding the mutable default argument.
In `@Deeploy/Targets/XDNA2/CodeTransformationPasses/MLIRRuntimeSequencePass.py`:
- Around line 85-89: The current sequence awaits outputTasks but never frees
them and frees inputTasks before ensuring completion; change the order so both
inputTasks and outputTasks are awaited before calling aiex_d.dma_free_task on
them. Specifically, use aiex_d.dma_await_task(...) for inputTasks as well (or
otherwise ensure their completion), then call aiex_d.dma_free_task(...) on each
input task, and after awaiting each output task call aiex_d.dma_free_task(...)
on each output task; update the block handling outputTasks/inputTasks (and
consider the origin of issue_token) so freeing only happens after dma_await_task
confirms completion.
In `@DeeployTest/Platforms/XDNA2/main.cpp`:
- Around line 79-112: Wrap the main logic of main (the code that calls
read_instr_binary(), xrt::device, xrt::xclbin, device.register_xclbin,
xrt::hw_context and xrt::kernel) in a try-catch block that catches
std::exception (and optionally a catch-all) and prints a clear error message
including ex.what() to stderr (or via your logging mechanism) and returns a
non-zero exit code; ensure any thrown exceptions from read_instr_binary() or XRT
constructors are handled so the program exits gracefully with an informative
message.
- Around line 65-77: In read_instr_binary, validate the file size and read
result: use std::streampos/istream::pos_type for tellg, check for error (e.g.,
tellg()==-1) and throw if so, verify byte_size is a multiple of sizeof(uint32_t)
and throw a descriptive std::runtime_error if not aligned, and after file.read
confirm gcount/streams state to ensure the full byte_size was read and throw on
short reads or IO errors; update uses of byte_size to a safely cast size_t only
after validation.
In `@DeeployTest/testUtils/core/execution.py`:
- Around line 171-173: The current code appends "-v" whenever config.verbose >=
1 which breaks the Generic/host binary that doesn't accept flags; change the
condition to only add "-v" when the run is for the XDNA2 binary (e.g. check the
simulator/platform identifier used in this module — e.g. simulator == "XDNA2" or
platform == "XDNA2") so replace the existing guard with something like: only
append "-v" when config.verbose >= 1 AND the simulator/platform equals the XDNA2
identifier (leave the cmd and config.verbose symbols intact and reference
XDNA2/main.cpp as the binary that accepts -v).
In `@requirements-xdna.txt`:
- Around line 6-10: requirements-xdna.txt currently lists llvm-aie without a
version, causing nondeterministic installs from the nightly index; update the
dependency by replacing the unpinned "llvm-aie" entry with a specific pinned
version compatible with mlir_aie (for example
"llvm-aie==18.0.0.2024050100+59d494d3" or a released tag you verified), so the
file contains the exact version string for llvm-aie alongside mlir_aie==v1.2.1
to ensure reproducible, compatible builds.
In `@TargetLibraries/XDNA2/kernels/add.cc`:
- Around line 20-40: The vector loop in eltwise_vadd uses F = size / vec_factor
and drops trailing elements when size isn't a multiple of vec_factor; add a
scalar tail loop after the existing vector loop that processes indices from F *
vec_factor up to size, performing element-wise addition using the same types
(T_in/T_out) and either the pointer cursors (pA1/pB1/pC1) or direct indexing
(a[i], b[i], c[i]) so all remaining elements are handled.
---
Nitpick comments:
In `@CMakeLists.txt`:
- Around line 23-24: The CMake cache property list for the platform option is
missing "XDNA2", causing GUI/ccmake users to not see it as a valid choice;
update the set_property(CACHE platform PROPERTY STRINGS ...) call to include
"XDNA2" (and keep it consistent with the initial set(platform ...) default if
you want it selectable there too) so the platform dropdown contains XDNA2
alongside MemPool, SoftHier, QEMU, Siracusa, Siracusa_w_neureka, PULP-Open,
GAP9, Generic, and Snitch.
In `@Container/Dockerfile.deeploy-xdna`:
- Around line 14-35: Update the RUN layer that invokes apt-get to add
--no-install-recommends to both apt-get install -y invocations in the multi-step
RUN command (the line starting with "RUN apt-get update && apt-get install -y \"
and the subsequent "&& apt-get update && apt-get install -y \" sequence) so that
unnecessary recommended packages are not installed and image size is reduced;
keep the existing rm -rf /var/lib/apt/lists/* cleanup as-is.
- Line 12: The ENV line setting LLVM_INSTALL_DIR="nope" is ambiguous; update the
Dockerfile to either set a meaningful path or add an inline comment explaining
why "nope" is used (e.g., as a deliberate sentinel to disable LLVM lookup or to
work around a specific build issue). Locate the ENV LLVM_INSTALL_DIR declaration
and either replace "nope" with the correct install path or append a short
comment describing the reason and any related workarounds or downstream
expectations so future maintainers aren’t confused.
In `@Deeploy/Targets/XDNA2/CodeTransformationPasses/__init__.py`:
- Around line 5-7: Replace the three wildcard imports in __init__.py with
explicit imports of the public symbols exported by MLIRComputeCorePass,
MLIRObjectFifoPass, and MLIRRuntimeSequencePass (e.g., import the specific
classes/functions you intend to re-export) and then define an explicit __all__
list containing those symbol names; update the import lines referencing
Deeploy.Targets.XDNA2.CodeTransformationPasses.MLIRComputeCorePass,
MLIRObjectFifoPass, and MLIRRuntimeSequencePass to import only the public
symbols and set __all__ = [...names...] so IDEs and static analyzers can
discover the package API.
In `@Deeploy/Targets/XDNA2/CodeTransformationPasses/MLIRComputeCorePass.py`:
- Around line 68-88: The code assumes a single representative tile type (tileTy
derived from firstKey) and reuses one subviewTy for all FIFO acquisitions, which
breaks when tensors have different types; update the loop so you look up the
per-key memref type via mlirBlock.fifoTypes[key] and create a matching subview
type for each acquisition (i.e., compute subviewTyPerKey from
mlirBlock.fifoTypes[key] before calling
aie_d.objectfifo_acquire/objectfifo_subview_access), handling inputs
(self.inputTensorKeys) and outputs (self.outputTensorKeys) independently rather
than reusing tileTy/subviewTy for every key.
In `@Deeploy/Targets/XDNA2/CodeTransformationPasses/MLIRObjectFifoPass.py`:
- Line 47: Replace the inefficient list(...) indexing when obtaining the first
key from outputConstraints: locate the assignment to firstOutputName in
MLIRObjectFifoPass.py and use next(iter(outputConstraints)) (or
next(iter(outputConstraints.keys()))) instead of
list(outputConstraints.keys())[0] so you avoid creating an intermediate list;
update the code where firstOutputName is set accordingly.
In `@Deeploy/Targets/XDNA2/Deployer.py`:
- Around line 159-171: The code derives tensorTy from the first block
(mlirBlocks[0]) using firstEb.numElements and a hardcoded ir.BF16Type, which
assumes every runtime-sequence arg shares the same element count and type;
update the runtime-sequence setup in Deployer.py to validate that all blocks in
mlirBlocks have matching numElements (and, if applicable, matching element
types) before creating tensorTy (or document this assumption clearly), and emit
a clear error or fallback if a mismatch is detected so
aiex_d.runtime_sequence(_seq) is not created with inconsistent tensor shapes.
In `@Deeploy/Targets/XDNA2/Platform.py`:
- Around line 126-138: Extract the duplicated engine-assignment logic from both
MemoryXDNA2Platform.getTargetMemoryLevel and
MemoryXDNA2PlatformWrapper.getTargetMemoryLevel into a shared helper (e.g.,
_get_xdna2_target_memory_level) that accepts the node and the default level
name; have each getTargetMemoryLevel call this helper instead of repeating the
hasattr/instanceof checks against XDNA2AIECoreEngine and preferredMemoryLevel,
returning engine.preferredMemoryLevel when present or
defaultTargetMemoryLevel.name otherwise.
In `@DeeployTest/generateNetwork_xdna2.py`:
- Around line 191-196: The golden-output computation currently hardcodes
elementwise addition using bf16_inputs -> bf16_input_f32 -> golden_f32 and will
fail for non-add ops; replace this with an operation-agnostic approach: either
run the ONNX graph (using the BF16-quantized inputs bf16_inputs) to produce
test_outputs_bf16, or inspect the model graph to extract the root operator(s)
and dispatch to the corresponding NumPy routine (e.g., use op_type from nodes
and call the appropriate handler) so that test_outputs_bf16 is produced
generically instead of summing elements.
- Around line 129-137: In the loop that currently reads "for index, (name,
values) in enumerate(zip(inputs_npz.files, test_inputs_f32))", mark the unused
filename variable as _ and make zip strict to avoid length-mismatch bugs: change
the header to "for index, (_, values) in enumerate(zip(inputs_npz.files,
test_inputs_f32, strict=True))" so you still populate inputTypes/inputOffsets
with PointerClass(bfloat16_t) as before; no other logic changes needed.
In `@DeeployTest/Platforms/XDNA2/CMakeLists.txt`:
- Around line 29-69: The XRT_INSTALL_DIR variable should be declared as a cache
variable like LLVM_AIE_INSTALL_DIR and MLIR_AIE_INSTALL_DIR for consistent
configuration and overrides; change the declaration so XRT_INSTALL_DIR is
initialized with set(XRT_INSTALL_DIR "$ENV{XRT_INSTALL_DIR}" CACHE PATH "XRT
install dir") (or similar) and preserve the existing fallback logic in the
if(NOT XRT_INSTALL_DIR) block that checks ENV{XILINX_XRT} and defaults to
"/opt/xilinx/xrt", ensuring the cache variable is used/checked instead of an
undeclared plain variable.
In `@DeeployTest/test_platforms.py`:
- Around line 128-150: Update the markers summary comment in the Platform
markers section of DeeployTest/test_platforms.py to include the new platform
marker `xdna2`; specifically add `xdna2: tests from the XDNA2 platform` (or a
similarly descriptive phrase) alongside the existing platform marker lines so
the comment accurately documents the new `xdna2` marker.
In `@DeeployTest/testUtils/platformMapping.py`:
- Around line 286-287: The except ImportError handler in the function that loads
deployers currently raises RuntimeError(f"Deployer for platform {platform} is
not implemented") without chaining the original ImportError; modify the except
block to capture the ImportError (e.g., except ImportError as e:) and re-raise
the RuntimeError using exception chaining (raise RuntimeError(f"Deployer for
platform {platform} is not implemented") from e) so the original traceback is
preserved for debugging.
In `@DeeployTest/testUtils/testRunner.py`:
- Around line 95-114: The CLI accepts invalid tiling inputs; validate them at
argument-parse time by replacing the raw int/str types with validation helpers:
for '--l1' and '--l3' use a positive-integer validator (e.g., positive_int) that
raises argparse.ArgumentTypeError for <=0 values, and for '--defaultMemLevel'
restrict values via choices (e.g., ['L1','L3']) or a validator that normalizes
and rejects unknown values; update the self.add_argument calls for l1, l3, and
defaultMemLevel in testRunner.py accordingly so parsing fails fast with clear
messages.
In `@README_XDNA.md`:
- Line 21: Update the README_XDNA.md sentence that embeds a personal developer
path by either removing the specific `/scratch/jungvi/IRON` example or replacing
it with a generic, documented workaround: describe that mounting an IRON repo is
a temporary step only required until Deeploy midend/backend support MLIR, show a
generic volume example like `-v /path/to/IRON:/opt/IRON`, and explain the
corresponding env var `IRON_OPERATORS_DIR=/opt/IRON/iron/operators` and when
users should use it versus the standard flow; ensure the text clearly labels it
as a temporary, developer-only workaround.
- Around line 6-8: Add language specifiers (e.g., bash or shell) to the fenced
code blocks that contain shell commands so they get proper syntax highlighting;
for example, change the block around the docker build command (`docker build -f
Container/Dockerfile.deeploy-xdna -t deeploy-xdna:local .`) to start with
```bash and similarly update the other command/code blocks referenced (lines
with commands at the other snippets) to use ```bash or ```shell so all shell
snippets in README_XDNA.md are consistently labeled.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e910d040-c1e1-4193-9705-285039a5ece9
📒 Files selected for processing (36)
.github/workflows/_runner-xdna2.yml.github/workflows/ci-platform-xdna2.yml.gitignoreCMakeLists.txtContainer/Dockerfile.deeploy-xdnaDeeploy/MLIRDataTypes.pyDeeploy/Targets/XDNA2/Bindings.pyDeeploy/Targets/XDNA2/CodeTransformationPasses/MLIRComputeCorePass.pyDeeploy/Targets/XDNA2/CodeTransformationPasses/MLIRObjectFifoPass.pyDeeploy/Targets/XDNA2/CodeTransformationPasses/MLIRRuntimeSequencePass.pyDeeploy/Targets/XDNA2/CodeTransformationPasses/__init__.pyDeeploy/Targets/XDNA2/Deployer.pyDeeploy/Targets/XDNA2/Parsers.pyDeeploy/Targets/XDNA2/Platform.pyDeeploy/Targets/XDNA2/Templates/AddTemplate.pyDeeploy/Targets/XDNA2/Templates/__init__.pyDeeploy/Targets/XDNA2/Tiler.pyDeeploy/Targets/XDNA2/TypeCheckers.pyDeeployTest/Platforms/XDNA2/CMakeLists.txtDeeployTest/Platforms/XDNA2/main.cppDeeployTest/Tests/Kernels/BF16/Add/Regular/inputs.npzDeeployTest/Tests/Kernels/BF16/Add/Regular/network.onnxDeeployTest/Tests/Kernels/BF16/Add/Regular/outputs.npzDeeployTest/conftest.pyDeeployTest/deeployRunner_xdna2.pyDeeployTest/generateNetwork_xdna2.pyDeeployTest/testUtils/core/execution.pyDeeployTest/testUtils/deeployRunner.pyDeeployTest/testUtils/platformMapping.pyDeeployTest/testUtils/testRunner.pyDeeployTest/test_platforms.pyDeeployTest/test_xdna2_config.pyREADME_XDNA.mdTargetLibraries/XDNA2/CMakeLists.txtTargetLibraries/XDNA2/kernels/add.ccrequirements-xdna.txt
Xeratec
requested changes
May 7, 2026
Member
Xeratec
left a comment
Xeratec
left a comment
There was a problem hiding this comment.
Hi Victor, thanks for this valuable contribution. I have some comments but overall the PR is already in a very good shape.
| docker_image: | ||
| description: "XDNA2 Docker image (must be pre-built on the runner)" | ||
| required: false | ||
| default: "deeploy-xdna:local" |
Member
There was a problem hiding this comment.
Why use the local file and not ghcr.io/pulp-platform/deeploy-xdnba:devel"?
Member
Author
There was a problem hiding this comment.
Because right now the CI runs locally on one of my machine and I don't have collaborators. Once this platform becomes more mainstream I think it will be a good idea to publish a docker but until then it's overkill IMO. Especially considering that the XDNA platform has little dependencies.
Member
There was a problem hiding this comment.
Hmm, okay, I do not fully agree,' as building the container and making it available to others is very easy. But if you insist, I wont prevent the merge because of this.
|
|
||
| def __init__(self, | ||
| name: str = "XDNA2_AIE_Core", | ||
| Mapping = XDNA2Mapping, |
Member
There was a problem hiding this comment.
Why does this use XDNA2Mapping and not XDNA2TilingMapping?
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (6)
Container/Dockerfile.deeploy-xdna (6)
43-45: ⚡ Quick winRemove unnecessary
/app/builddirectory creation and cleanup.Lines 14–15 create
/app/buildas a WORKDIR, but no build operations actually occur there before it's removed at line 45. This cleanup is pointless and confusing. Either use/app/buildfor actual build operations (as suggested in past reviews), or skip creating it entirely.♻️ Proposed fix to remove the unnecessary steps
-WORKDIR /app/build - RUN apt-get update && apt-get install -y \ software-properties-common \ && add-apt-repository -y ppa:amd-team/xrt \-# Remove unused files and clean up to reduce image size -WORKDIR /app -RUN rm -rf /app/build - COPY pyproject.toml requirements-xdna.txt ./And add
WORKDIR /appbefore the COPY command:+WORKDIR /app + COPY pyproject.toml requirements-xdna.txt ./🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Container/Dockerfile.deeploy-xdna` around lines 43 - 45, Remove the pointless creation and cleanup of /app/build: delete the WORKDIR /app/build and the RUN rm -rf /app/build lines (they are the WORKDIR /app/build and RUN rm -rf /app/build entries) and ensure a single WORKDIR /app is set before the COPY commands so files are copied into /app directly; alternatively, if you intend to perform builds in a separate context, convert the created WORKDIR /app/build into the actual build step instead of removing it.
41-41: 💤 Low valueAppend to
LD_LIBRARY_PATHinstead of replacing it.The current assignment replaces any existing
LD_LIBRARY_PATHvalue. For better composability with base images or multi-stage builds, append the XRT library path instead.🔗 Proposed fix to append the path
-ENV LD_LIBRARY_PATH=${XILINX_XRT}/lib +ENV LD_LIBRARY_PATH=${XILINX_XRT}/lib:${LD_LIBRARY_PATH}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Container/Dockerfile.deeploy-xdna` at line 41, The Dockerfile currently sets LD_LIBRARY_PATH with ENV LD_LIBRARY_PATH=${XILINX_XRT}/lib which overwrites any existing LD_LIBRARY_PATH; change it to append the XRT lib path instead so existing entries are preserved. Update the ENV instruction that references LD_LIBRARY_PATH and XILINX_XRT to export LD_LIBRARY_PATH=${LD_LIBRARY_PATH:+$LD_LIBRARY_PATH:}${XILINX_XRT}/lib (or equivalent shell-safe append), ensuring it handles an empty original value correctly and uses the existing LD_LIBRARY_PATH if present.
56-56: ⚖️ Poor tradeoffConsider adding a non-root USER for security.
The container runs as root by default, which is flagged by static analysis (DS-0002) as a security concern. While root access may be necessary for NPU device access (
/dev/accel/accel0) in the CI environment, consider whether a non-root user with appropriate device permissions would work.If root is required, document why in a comment. Otherwise, add a USER directive:
🔒 Proposed fix to add non-root user
ENV MLIR_AIE_PYTHON=/usr/bin/python3 +# Create non-root user for security +RUN useradd -m -s /bin/bash deeploy && \ + chown -R deeploy:deeploy /app +USER deeploy + WORKDIR /app/DeeployNote: If NPU device access requires root, this may not be feasible without additional device permission configuration.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Container/Dockerfile.deeploy-xdna` at line 56, The Dockerfile currently sets WORKDIR /app/Deeploy and leaves the container running as root; add a non-root user (e.g., create and chown to a user) and add a USER directive after WORKDIR so the app runs without root, or if NPU device access truly requires root, add an explicit comment explaining why root is required and list the device (/dev/accel/accel0) and CI constraints; update ownership of /app/Deeploy to that user and ensure any startup scripts or entries in the Dockerfile (where WORKDIR is used) run under that user.
48-52: ⚡ Quick winRemove
toml-to-requirementsafter use to reduce image size.The
toml-to-requirementspackage is installed to generaterequirements.txtbut is left in the image afterward. Since it's only needed at build time, uninstall it to reduce the final image size.🗑️ Proposed fix to uninstall build-time dependency
COPY pyproject.toml requirements-xdna.txt ./ RUN pip install toml-to-requirements && \ toml-to-req --toml-file pyproject.toml && \ pip install -r requirements.txt && \ pip install -r requirements-xdna.txt && \ + pip uninstall -y toml-to-requirements && \ rm -f requirements.txt pyproject.toml requirements-xdna.txt🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Container/Dockerfile.deeploy-xdna` around lines 48 - 52, The Dockerfile currently installs the build-only package toml-to-requirements and leaves it in the image; update the RUN sequence that calls pip install toml-to-requirements and toml-to-req so that after generating requirements.txt and installing the requirements you uninstall the build tool (e.g., add a pip uninstall -y toml-to-requirements step after pip install -r requirements.txt && pip install -r requirements-xdna.txt) and then remove the temporary files (requirements.txt, pyproject.toml, requirements-xdna.txt) so the build-time dependency is not retained in the final image.
16-37: ⚡ Quick winAdd
--no-install-recommendsto reduce image size.The
apt-get installcommands on lines 19 and 19 (second invocation) are missing the--no-install-recommendsflag, which causes APT to install recommended packages and increases the image size unnecessarily.📦 Proposed fix to add the flag
RUN apt-get update && apt-get install -y \ + --no-install-recommends \ software-properties-common \ && add-apt-repository -y ppa:amd-team/xrt \ && apt-get update && apt-get install -y \ + --no-install-recommends \ cmake \ ninja-build \🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Container/Dockerfile.deeploy-xdna` around lines 16 - 37, Update the two apt-get install invocations in the RUN instruction so they include the --no-install-recommends flag to avoid pulling recommended packages and reduce image size; modify the RUN block that calls "apt-get install -y software-properties-common" and the subsequent "apt-get install -y cmake ninja-build g++ ..." to add --no-install-recommends to each apt-get install command and keep the existing && rm -rf /var/lib/apt/lists/* cleanup unchanged.
12-12: 💤 Low valueReplace
LLVM_INSTALL_DIR="nope"with a valid value or remove it.The environment variable
LLVM_INSTALL_DIRis used by the test suite to locate the toolchain installation directory. Setting it to"nope"is not a valid path and will cause test failures when the code attempts to access files in this directory. Either leave it unset (allowing tests to skip gracefully) or set it to the actual path where the LLVM toolchain is installed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Container/Dockerfile.deeploy-xdna` at line 12, The Dockerfile sets ENV LLVM_INSTALL_DIR="nope", which is an invalid path and breaks tests; update the ENV LLVM_INSTALL_DIR entry to either remove it entirely or set it to a valid LLVM toolchain install path (replace the value for LLVM_INSTALL_DIR with the actual installation directory used in your environment or omit the ENV line so tests can skip), ensuring any references to LLVM_INSTALL_DIR in the build/test scripts will resolve correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@Container/Dockerfile.deeploy-xdna`:
- Around line 43-45: Remove the pointless creation and cleanup of /app/build:
delete the WORKDIR /app/build and the RUN rm -rf /app/build lines (they are the
WORKDIR /app/build and RUN rm -rf /app/build entries) and ensure a single
WORKDIR /app is set before the COPY commands so files are copied into /app
directly; alternatively, if you intend to perform builds in a separate context,
convert the created WORKDIR /app/build into the actual build step instead of
removing it.
- Line 41: The Dockerfile currently sets LD_LIBRARY_PATH with ENV
LD_LIBRARY_PATH=${XILINX_XRT}/lib which overwrites any existing LD_LIBRARY_PATH;
change it to append the XRT lib path instead so existing entries are preserved.
Update the ENV instruction that references LD_LIBRARY_PATH and XILINX_XRT to
export LD_LIBRARY_PATH=${LD_LIBRARY_PATH:+$LD_LIBRARY_PATH:}${XILINX_XRT}/lib
(or equivalent shell-safe append), ensuring it handles an empty original value
correctly and uses the existing LD_LIBRARY_PATH if present.
- Line 56: The Dockerfile currently sets WORKDIR /app/Deeploy and leaves the
container running as root; add a non-root user (e.g., create and chown to a
user) and add a USER directive after WORKDIR so the app runs without root, or if
NPU device access truly requires root, add an explicit comment explaining why
root is required and list the device (/dev/accel/accel0) and CI constraints;
update ownership of /app/Deeploy to that user and ensure any startup scripts or
entries in the Dockerfile (where WORKDIR is used) run under that user.
- Around line 48-52: The Dockerfile currently installs the build-only package
toml-to-requirements and leaves it in the image; update the RUN sequence that
calls pip install toml-to-requirements and toml-to-req so that after generating
requirements.txt and installing the requirements you uninstall the build tool
(e.g., add a pip uninstall -y toml-to-requirements step after pip install -r
requirements.txt && pip install -r requirements-xdna.txt) and then remove the
temporary files (requirements.txt, pyproject.toml, requirements-xdna.txt) so the
build-time dependency is not retained in the final image.
- Around line 16-37: Update the two apt-get install invocations in the RUN
instruction so they include the --no-install-recommends flag to avoid pulling
recommended packages and reduce image size; modify the RUN block that calls
"apt-get install -y software-properties-common" and the subsequent "apt-get
install -y cmake ninja-build g++ ..." to add --no-install-recommends to each
apt-get install command and keep the existing && rm -rf /var/lib/apt/lists/*
cleanup unchanged.
- Line 12: The Dockerfile sets ENV LLVM_INSTALL_DIR="nope", which is an invalid
path and breaks tests; update the ENV LLVM_INSTALL_DIR entry to either remove it
entirely or set it to a valid LLVM toolchain install path (replace the value for
LLVM_INSTALL_DIR with the actual installation directory used in your environment
or omit the ENV line so tests can skip), ensuring any references to
LLVM_INSTALL_DIR in the build/test scripts will resolve correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 980abba0-cc9f-4991-a904-aa73c9587cd5
📒 Files selected for processing (5)
Container/Dockerfile.deeploy-xdnaDeeploy/Targets/XDNA2/CodeTransformationPasses/MLIRRuntimeSequencePass.pyDeeployTest/testUtils/core/execution.pyTargetLibraries/XDNA2/kernels/add.ccrequirements-xdna.txt
✅ Files skipped from review due to trivial changes (1)
- requirements-xdna.txt
🚧 Files skipped from review as they are similar to previous changes (2)
- DeeployTest/testUtils/core/execution.py
- Deeploy/Targets/XDNA2/CodeTransformationPasses/MLIRRuntimeSequencePass.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/_runner-xdna2.yml:
- Around line 31-32: Validate and safely quote workflow inputs before they are
used: reject or fail fast if inputs.docker-image or inputs.pytest-marker contain
unsafe characters (eg. leading '-' or shell metacharacters) by testing them
against a strict whitelist regex (alphanumeric, ./:_@- and optional tag), then
use quoted interpolations everywhere they are used (e.g. "${{
inputs.docker-image }}" in the docker run/exec command and "${{
inputs.pytest-marker }}" when calling pytest inside bash) and avoid passing
unvalidated values directly into shell or docker arguments (also ensure any
chown or other shell lines use quoted paths and &&/|| chaining safely instead of
relying on interpolation). Ensure the validation step is implemented early in
the job and fails the run with a clear error if inputs do not match the allowed
pattern.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 929e79f9-1673-41e0-abe3-b43f9e0231ed
📒 Files selected for processing (1)
.github/workflows/_runner-xdna2.yml
Comment on lines
+31
to
+32
| ${{ inputs.docker-image }} \ | ||
| chown -R $(id -u):$(id -g) /workspace || true |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify all call sites and current constraints for reusable workflow inputs
set -euo pipefail
rg -n -C2 'uses:\s*\./\.github/workflows/_runner-xdna2\.yml|pytest-marker:|docker-image:' .github/workflows
rg -n -C2 'inputs:\s*$|pytest-marker:|docker-image:' .github/workflows/_runner-xdna2.ymlRepository: pulp-platform/Deeploy
Length of output: 20728
🏁 Script executed:
cat -n .github/workflows/_runner-xdna2.ymlRepository: pulp-platform/Deeploy
Length of output: 1914
Validate and quote workflow inputs before using in docker and bash commands.
inputs.docker-image (lines 31, 48) is unquoted and directly interpolated as a docker image argument, allowing injection of docker flags (e.g., -v /etc:/etc). inputs.pytest-marker (line 52) is passed unvalidated to pytest inside bash. On a self-hosted runner with --device /dev/accel/accel0 and volume mounts to /opt/xilinx, this enables command injection with privileged access.
Add input validation before use and quote all interpolations:
Suggested hardening
jobs:
test-runner-xdna2:
runs-on: xdna2-npu
steps:
+ - name: Validate workflow inputs
+ shell: bash
+ env:
+ DOCKER_IMAGE: ${{ inputs.docker-image }}
+ PYTEST_MARKER_EXPR: ${{ inputs.pytest-marker }}
+ run: |
+ set -euo pipefail
+ [[ "$DOCKER_IMAGE" =~ ^[a-zA-Z0-9._/:`@-`]+$ ]] || { echo "Invalid docker-image"; exit 1; }
+ [[ "$PYTEST_MARKER_EXPR" =~ ^[a-zA-Z0-9_[:space:]()!-]+$ ]] || { echo "Invalid pytest-marker"; exit 1; }
+
- name: Fix workspace permissions
shell: bash
run: |
docker run --rm \
-v "${{ github.workspace }}":/workspace \
- ${{ inputs.docker-image }} \
+ "${{ inputs.docker-image }}" \
chown -R $(id -u):$(id -g) /workspace || true
- name: Run Tests in Docker
shell: bash
+ env:
+ PYTEST_MARKER_EXPR: ${{ inputs.pytest-marker }}
run: |
docker run --rm \
--device /dev/accel/accel0 \
--ulimit memlock=-1 \
-v /opt/xilinx:/opt/xilinx \
-v "${{ github.workspace }}":/app/Deeploy \
-w /app/Deeploy \
- ${{ inputs.docker-image }} \
+ "${{ inputs.docker-image }}" \
bash -c "
pip install -e . &&
cd DeeployTest &&
- pytest test_platforms.py -v -m 'xdna2 and ${{ inputs.pytest-marker }}'
+ pytest test_platforms.py -v -m \"xdna2 and ${PYTEST_MARKER_EXPR}\"
"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/_runner-xdna2.yml around lines 31 - 32, Validate and
safely quote workflow inputs before they are used: reject or fail fast if
inputs.docker-image or inputs.pytest-marker contain unsafe characters (eg.
leading '-' or shell metacharacters) by testing them against a strict whitelist
regex (alphanumeric, ./:_@- and optional tag), then use quoted interpolations
everywhere they are used (e.g. "${{ inputs.docker-image }}" in the docker
run/exec command and "${{ inputs.pytest-marker }}" when calling pytest inside
bash) and avoid passing unvalidated values directly into shell or docker
arguments (also ensure any chown or other shell lines use quoted paths and &&/||
chaining safely instead of relying on interpolation). Ensure the validation step
is implemented early in the job and fails the run with a clear error if inputs
do not match the allowed pattern.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
TargetLibraries/XDNA2/CMakeLists.txt (1)
29-29: ⚡ Quick winAdd
CONFIGURE_DEPENDStofile(GLOB ...)to avoid stale builds when new kernels are added.Without
CONFIGURE_DEPENDS, adding a new*.cctokernels/won't trigger a CMake reconfigure, silently omitting the new kernel from the build.♻️ Proposed fix
-file(GLOB XDNA2_KERNEL_SOURCES "${CMAKE_CURRENT_LIST_DIR}/kernels/*.cc") +file(GLOB XDNA2_KERNEL_SOURCES CONFIGURE_DEPENDS "${CMAKE_CURRENT_LIST_DIR}/kernels/*.cc")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@TargetLibraries/XDNA2/CMakeLists.txt` at line 29, The file(GLOB ...) invocation that defines XDNA2_KERNEL_SOURCES should be updated to include the CONFIGURE_DEPENDS option so CMake will re-run configuration when files under kernels/ change; locate the file(GLOB XDNA2_KERNEL_SOURCES "${CMAKE_CURRENT_LIST_DIR}/kernels/*.cc") line in CMakeLists.txt and add CONFIGURE_DEPENDS to that file(GLOB) call to ensure new *.cc kernel files are detected and the build is not silently stale.Deeploy/Targets/XDNA2/Platform.py (1)
118-149: ⚡ Quick win
getTargetMemoryLevelis duplicated verbatim in bothMemoryXDNA2PlatformandMemoryXDNA2PlatformWrapper.Extract the shared logic to a module-level helper. The
hasattr(engine, 'preferredMemoryLevel')guard is also alwaysTrueforXDNA2AIECoreEngine(it's set unconditionally in__init__), so it can be dropped.♻️ Proposed refactor
+def _xdna2_target_memory_level(node: gs.Node, default: MemoryLevel) -> str: + """Return the AIE core's preferred memory level, or the platform default.""" + if hasattr(node, '_engine_assignment'): + engine = node._engine_assignment + if isinstance(engine, XDNA2AIECoreEngine): + return engine.preferredMemoryLevel + return default.name + class MemoryXDNA2Platform(MemoryPlatform): ... def getTargetMemoryLevel(self, node: gs.Node, tensorName: str, ctxt: NetworkContext) -> str: - if hasattr(node, '_engine_assignment'): - engine = node._engine_assignment - if isinstance(engine, XDNA2AIECoreEngine) and hasattr(engine, 'preferredMemoryLevel'): - return engine.preferredMemoryLevel - return self.defaultTargetMemoryLevel.name + return _xdna2_target_memory_level(node, self.defaultTargetMemoryLevel) class MemoryXDNA2PlatformWrapper(MemoryPlatformWrapper): ... def getTargetMemoryLevel(self, node: gs.Node, tensorName: str, ctxt: NetworkContext) -> str: - if hasattr(node, '_engine_assignment'): - engine = node._engine_assignment - if isinstance(engine, XDNA2AIECoreEngine) and hasattr(engine, 'preferredMemoryLevel'): - return engine.preferredMemoryLevel - return self.defaultTargetMemoryLevel.name + return _xdna2_target_memory_level(node, self.defaultTargetMemoryLevel)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Deeploy/Targets/XDNA2/Platform.py` around lines 118 - 149, Both MemoryXDNA2Platform.getTargetMemoryLevel and MemoryXDNA2PlatformWrapper.getTargetMemoryLevel duplicate the same logic; extract that shared logic into a module-level helper (e.g., resolve_target_memory_level(engine, default_level)) and have both methods call it. In the helper use isinstance(engine, XDNA2AIECoreEngine) to return engine.preferredMemoryLevel and otherwise return default_level.name; drop the redundant hasattr(engine, 'preferredMemoryLevel') guard because XDNA2AIECoreEngine always defines it. Update references in getTargetMemoryLevel in both classes to call the new helper and remove the duplicated code.DeeployTest/generateNetwork_xdna2.py (1)
58-111: ⚡ Quick win
_generate_xdna2_inputs_headerand_generate_xdna2_outputs_headerare near-identical.The only differences are the
"Input"/"Output"labels and"input"/"output"macro suffixes. Merging them into one helper avoids duplicate bug-surface.♻️ Proposed refactor
-def _generate_xdna2_inputs_header(input_arrays: list) -> str: - """Generate testinputs.h with raw uint16_t BF16 bit-pattern arrays.""" - ... - -def _generate_xdna2_outputs_header(output_arrays: list) -> str: - """Generate testoutputs.h with raw uint16_t BF16 bit-pattern arrays.""" - ... +def _generate_xdna2_tensor_header(arrays: list, kind: str) -> str: + """Generate a BF16 uint16_t header for *kind* ('input' or 'output').""" + Kind = kind.capitalize() + lines = [] + lines.append("// SPDX-FileCopyrightText: 2026 ETH Zurich and University of Bologna") + lines.append("// SPDX-License-Identifier: Apache-2.0") + lines.append("// Auto-generated by generateNetwork_xdna2.py — do not edit.") + lines.append("#pragma once") + lines.append("#include <stdint.h>") + lines.append("") + vec_names = [] + for idx, arr in enumerate(arrays): + bf16 = _float32_to_bf16_uint16(arr.flatten()) + n = len(bf16) + name = f"test{Kind}Vector{idx}" + vec_names.append(name) + hex_vals = ", ".join(f"0x{v:04x}u" for v in bf16) + lines.append(f"static const uint16_t {name}[{n}] = {{{hex_vals}}};") + lines.append(f"#define N_ELEMENTS_{kind.upper()}{idx} {n}u") + lines.append("") + lines.append(f"static const void *test{Kind}Vector[{len(vec_names)}] = {{") + lines.append(" " + ", ".join(f"(const void *){n}" for n in vec_names)) + lines.append("};") + lines.append("") + return "\n".join(lines)Call sites:
testInputStr = _generate_xdna2_tensor_header(test_inputs_f32, "input") testOutputStr = _generate_xdna2_tensor_header(test_outputs_bf16, "output")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@DeeployTest/generateNetwork_xdna2.py` around lines 58 - 111, Both _generate_xdna2_inputs_header and _generate_xdna2_outputs_header are duplicated; replace them with a single parameterized helper (e.g. _generate_xdna2_tensor_header(arrays: list, kind: str)) that takes the arrays and a kind token ("input" or "output") and emits the same content but using kind to form names like testInputVector/testOutputVector, N_ELEMENTS_INPUT#/N_ELEMENTS_OUTPUT#, and testInputVector/testOutputVector table; update call sites that use _generate_xdna2_inputs_header and _generate_xdna2_outputs_header to call the new _generate_xdna2_tensor_header with the appropriate kind string, reusing the same internal logic (flatten -> _float32_to_bf16_uint16, name generation, hex formatting, and final pointer array construction).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@DeeployTest/generateNetwork_xdna2.py`:
- Line 61: The autogenerated header strings in _generate_xdna2_inputs_header and
_generate_xdna2_outputs_header embed the wrong copyright year (2025); update the
hardcoded year in the string appended (the line that builds "//
SPDX-FileCopyrightText: 2025 ...") to 2026 in both functions so the generated
files show 2026 consistently.
- Line 129: Rename the unused loop variable name to _name and add strict=True to
the zip call to satisfy Ruff warnings; locate the loop using
enumerate(zip(inputs_npz.files, test_inputs_f32)) in generateNetwork_xdna2.py
(the loop that iterates over inputs_npz.files and test_inputs_f32) and change it
so the zip is called with strict=True and the tuple unpacks to (_name, values)
so the unused first element is prefixed with an underscore.
In `@DeeployTest/Platforms/XDNA2/CMakeLists.txt`:
- Around line 71-73: Replace the hardcoded "python" invocation in the CMake
COMMAND that runs AIECC (the COMMAND using ${CMAKE_COMMAND} -E env with
"PATH=${MLIR_AIE_INSTALL_DIR}/bin:$ENV{PATH}" "python" "${AIECC_PY}") with the
resolved Python 3 executable variable ${Python3_EXECUTABLE}; locate the COMMAND
in CMakeLists.txt that references AIECC_PY and swap the "python" token for
${Python3_EXECUTABLE} so the script runs with the intended Python 3 interpreter.
In `@README_XDNA.md`:
- Around line 6-8: Add language identifiers (bash) to all fenced code blocks in
README_XDNA.md: update the docker build command block ("docker build -f
Container/Dockerfile.deeploy-xdna -t deeploy-xdna:local ."), the docker run
block (the multi-line run with --device/--ulimit/-v/--name), the pip install +
python deeployRunner_xdna2.py block, the indented CI/example docker build
snippet, the actions-runner setup block (mkdir actions-runner, ./config.sh --url
... --labels xdna2-npu), and the svc.sh commands (sudo ./svc.sh install/start)
by changing each opening ``` to ```bash so markdownlint MD040 is satisfied.
---
Nitpick comments:
In `@Deeploy/Targets/XDNA2/Platform.py`:
- Around line 118-149: Both MemoryXDNA2Platform.getTargetMemoryLevel and
MemoryXDNA2PlatformWrapper.getTargetMemoryLevel duplicate the same logic;
extract that shared logic into a module-level helper (e.g.,
resolve_target_memory_level(engine, default_level)) and have both methods call
it. In the helper use isinstance(engine, XDNA2AIECoreEngine) to return
engine.preferredMemoryLevel and otherwise return default_level.name; drop the
redundant hasattr(engine, 'preferredMemoryLevel') guard because
XDNA2AIECoreEngine always defines it. Update references in getTargetMemoryLevel
in both classes to call the new helper and remove the duplicated code.
In `@DeeployTest/generateNetwork_xdna2.py`:
- Around line 58-111: Both _generate_xdna2_inputs_header and
_generate_xdna2_outputs_header are duplicated; replace them with a single
parameterized helper (e.g. _generate_xdna2_tensor_header(arrays: list, kind:
str)) that takes the arrays and a kind token ("input" or "output") and emits the
same content but using kind to form names like testInputVector/testOutputVector,
N_ELEMENTS_INPUT#/N_ELEMENTS_OUTPUT#, and testInputVector/testOutputVector
table; update call sites that use _generate_xdna2_inputs_header and
_generate_xdna2_outputs_header to call the new _generate_xdna2_tensor_header
with the appropriate kind string, reusing the same internal logic (flatten ->
_float32_to_bf16_uint16, name generation, hex formatting, and final pointer
array construction).
In `@TargetLibraries/XDNA2/CMakeLists.txt`:
- Line 29: The file(GLOB ...) invocation that defines XDNA2_KERNEL_SOURCES
should be updated to include the CONFIGURE_DEPENDS option so CMake will re-run
configuration when files under kernels/ change; locate the file(GLOB
XDNA2_KERNEL_SOURCES "${CMAKE_CURRENT_LIST_DIR}/kernels/*.cc") line in
CMakeLists.txt and add CONFIGURE_DEPENDS to that file(GLOB) call to ensure new
*.cc kernel files are detected and the build is not silently stale.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ba7431d7-ddea-48c6-80e0-cebc4b8867f8
📒 Files selected for processing (21)
.github/workflows/_runner-xdna2.yml.github/workflows/ci-platform-xdna2.ymlDeeploy/Targets/XDNA2/Bindings.pyDeeploy/Targets/XDNA2/Deployer.pyDeeploy/Targets/XDNA2/Parsers.pyDeeploy/Targets/XDNA2/Platform.pyDeeploy/Targets/XDNA2/Templates/AddTemplate.pyDeeploy/Targets/XDNA2/Templates/__init__.pyDeeploy/Targets/XDNA2/Tiler.pyDeeploy/Targets/XDNA2/TypeCheckers.pyDeeployTest/Platforms/XDNA2/CMakeLists.txtDeeployTest/Platforms/XDNA2/main.cppDeeployTest/deeployRunner_xdna2.pyDeeployTest/generateNetwork_xdna2.pyDeeployTest/test_xdna2_config.pyREADME_XDNA.mdTargetLibraries/XDNA2/CMakeLists.txtTargetLibraries/XDNA2/kernels/add.cccmake/xdna2/llvm_aie.cmakecmake/xdna2/mlir_aie.cmakerequirements-xdna.txt
✅ Files skipped from review due to trivial changes (4)
- requirements-xdna.txt
- Deeploy/Targets/XDNA2/Templates/init.py
- cmake/xdna2/mlir_aie.cmake
- Deeploy/Targets/XDNA2/Parsers.py
🚧 Files skipped from review as they are similar to previous changes (7)
- DeeployTest/test_xdna2_config.py
- Deeploy/Targets/XDNA2/Tiler.py
- .github/workflows/ci-platform-xdna2.yml
- Deeploy/Targets/XDNA2/Bindings.py
- Deeploy/Targets/XDNA2/Templates/AddTemplate.py
- DeeployTest/deeployRunner_xdna2.py
- Deeploy/Targets/XDNA2/Deployer.py
Member
|
Thanks for implementing the changes! I think we are almost there. I resolved most of the comments. Please check the remaining ones, rebase your work, update the Changelog, and let Claude extend the PR description with changes, fixes, and added stuff. Once the CI passes, I can take another quick look and hopefully merge it. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR adds beta support for the XDNA2 platform. This is also the first attempt at adding a new MLIR backend for Deeploy.
Currently, it passes a single Add node ONNX graph; on this platform, there is no such "untitled" execution possible, as the AIE cores can't read outside their local L1. Hence, the XDNA platform is inherently tiled.
Currently, the CI uses self-hosted runners to run directly on the XDNA2 NPU from a Minisforum X1 Pro.
Added
README_XDNA.md.Changed
Fixed
PR Merge Checklist
develcommit and pointing todevel.CHANGELOG.mdfile has been updated.